Skip to content

Fix documented bugs (zero-BC): schema, joins, events, soft-delete opt-in#17

Open
abdul-kaioum wants to merge 32 commits into
refactor/namespace-grammarfrom
fix/documented-bugs
Open

Fix documented bugs (zero-BC): schema, joins, events, soft-delete opt-in#17
abdul-kaioum wants to merge 32 commits into
refactor/namespace-grammarfrom
fix/documented-bugs

Conversation

@abdul-kaioum

Copy link
Copy Markdown
Member

Fixes the code bugs documented in docs/usage.md (## Limitations & known issues) and docs/schema.md. Stacked on #16 (retargets to main when #16 merges).

Hard constraint: no existing plugin breaks

Every change is either (a) a repair of already-broken output (nothing working depended on it) or (b) additive / opt-in with the default preserving today's behavior byte-for-byte. Verified live + by review.

Tier A — pure bug fixes (already-broken / additive, zero-BC)

  • change()MODIFY COLUMN (was malformed ADD COLUMN CHANGE COLUMN).
  • Direct-call schema drop helpers emit full SQL (dropPrimary/Timestamps/Index/Unique/Foreign were no-ops outside Schema::edit()).
  • Subscribable creating/created events (registrars were missing; additive).
  • Bulk insert() always returns a Collection (error-path fallback returned a bare array).
  • Joins no longer double-prefix (wp_wp_*wp_*) and qualify the ON second column. Only the bug-locking GrammarTest::testJoin updates; dotted-column joins unchanged.

Tier B — behavior-changers, redesigned as opt-in (default = today)

  • Schema::withWpPrefix() opt-in — Schema::create('orders') still makes a bare orders table (changing it would orphan data); opt in for wp_orders.
  • Opt-in soft-delete read scopepublic $soft_delete_scope = true; enables auto deleted_at IS NULL on reads; default (flag unset) returns all rows, SQL byte-identical to today. Adds withTrashed() / onlyTrashed() scopes (the latter works on any soft-delete model). Scope injects on SELECT only (not delete/update), threads through aggregates/relations, and parenthesizes the user WHERE so it can't leak past orWhere.

Verification

  • 112 tests green (85 baseline + 27 new), php-cs-fixer + PHPCompatibility 7.4- clean, no PHPUnit deprecations.
  • TDD per fix (RED → fix → green); each fix its own commit; each removes/reframes its docs Limitations bullet.
  • Final review (momus): approve — zero-BC holds, no blockers. Non-scoped queries verified byte-identical.

Known follow-up (opt-in only)

On a $soft_delete_scope model, refresh() re-queries via a scoped SELECT, so reloading a trashed row then save()-ing INSERTs instead of UPDATEs. Affects only models that opt in; documented in docs/usage.md as a follow-up.

Deferred (not in this PR)

  • upsert updated_at = VALUES(created_at) mapping — a test locks the current mapping and the doc's suggested replacement is invalid SQL; needs a design decision (own PR).
  • belongsToMany pivot support — new feature, own spec.

🤖 Generated with Claude Code

Blueprint::addColumnQuery() unconditionally prepended ADD COLUMN in edit
mode and then also prepended CHANGE COLUMN when the change flag was set,
producing invalid SQL. Make the two prefixes mutually exclusive: a changed
column now emits MODIFY COLUMN; non-changed columns in edit mode still
emit ADD COLUMN. Add SchemaTest with a failing-first test for both paths.
Add has_cap() stub to FakeWpdb so Schema tests can boot. Remove the
change() Known bug note and Limitations bullets from docs/schema.md and
docs/usage.md.

Assisted-By: AI
Extract DROP-clause builders and a shared applyDropClause() dispatcher
so dropPrimary, dropTimestamps, dropIndex, dropUnique, and dropForeign
emit a complete ALTER TABLE … DROP … statement when called directly
(e.g. Schema::dropPrimary('orders')) instead of an empty header.
Edit-mode aggregation via Schema::edit() is unchanged. Remove the now-
invalid Limitations docs that documented these as edit-only helpers.

Assisted-By: AI
The fallback path in QueryBuilder::bulkInsert() — triggered when the
post-insert re-query hydrates nothing — returned a bare int[] instead of a
Collection, breaking the documented contract. Wrap $ids in new Collection($ids)
so callers receive a consistent Collection regardless of path.

Element-type caveat: Collection elements are Model instances on the happy path
(re-query succeeded) and ints (inserted IDs) on the fallback. foreach/count()
are unaffected; is_array()/empty() now return false on the fallback.

Docs: remove the Limitations bullet that documented the bug; update the
bulk-insert code comment to note the element-type distinction.

Assisted-By: AI
Adds a real static method Schema::withWpPrefix() that sets the Schema
prefix to Connection::getPrefix() (wp_ + plugin prefix), giving authors
a zero-BC way to create tables that match Model table names.

Default behaviour (bare table name) is unchanged: Schema::create('orders')
still emits CREATE TABLE IF NOT EXISTS orders. The BC guard test locks this.

Docs: reframe the prefix Limitations bullet in usage.md and schema.md from
"does not auto-apply" to "by design bare", and document the new method in
the Table operations table with a code example.

Assisted-By: AI
Prevents AND/OR precedence leak when orWhere is combined with the
auto-scope or onlyTrashed, which could expose trashed rows.

Assisted-By: AI

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several improvements and bug fixes to the database and schema builder, including an opt-in automatic soft-delete read scope with withTrashed() and onlyTrashed() methods, support for creating and created model events, and ensuring bulk insert() always returns a Collection. It also resolves table prefixing and alias issues in joins, refactors index dropping in Blueprint, and fixes the change() column modifier to correctly emit MODIFY COLUMN. The review feedback recommends enhancing the table alias parsing regex to handle arbitrary whitespace and adding a validation check before auto-prefixing the second column in prepareOn() to prevent syntax errors when comparing against constants or functions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/QueryBuilder.php
Comment on lines +770 to +772
$parts = preg_split('/ as /i', $table);
$rawTable = $parts[0];
$alias = isset($parts[1]) ? $parts[1] : null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The regular expression / as /i used to split the table and its alias is quite restrictive and can fail if there are multiple spaces or other whitespace characters (like tabs) around the as keyword.

Using \s+as\s+/i along with trim() is much more robust and standard for parsing SQL aliases.

        $parts         = preg_split('/\s+as\s+/i', trim($table));
        $rawTable      = $parts[0];
        $alias         = isset($parts[1]) ? $parts[1] : null;

Comment thread src/QueryBuilder.php
Comment on lines +1461 to +1463
if (!\is_null($secondColumn) && strpos($secondColumn, '.') === false) {
$secondColumn = $table . '.' . $secondColumn;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When preparing the ON clause, any string that does not contain a dot is automatically prefixed with the table name. However, if the second column is a constant value (e.g., a number, a quoted string like 'active'), or a function call (e.g., NOW()), this auto-prefixing will result in invalid SQL (e.g., posts.10 or posts.'active').

Adding a check to ensure the second column is a valid column identifier before prefixing it prevents these syntax errors.

        if (!\is_null($secondColumn) && strpos($secondColumn, '.') === false && preg_match('/^[a-zA-Z_][a-zA-Z0-9_]*$/', $secondColumn)) {
            $secondColumn = $table . '.' . $secondColumn;
        }

upsert() on a $timestamps model now inserts both created_at and updated_at and,
on a duplicate key, bumps updated_at (VALUES(updated_at)) while preserving the
original created_at — replacing the prior logic that left updated_at unmanaged
and emitted the `updated_at = VALUES(created_at)` swap. No timestamp magic when
$timestamps is false.

Splits the non-generic GeminiReviewFixesTest into topical suites — UpsertTest,
AttributeCastTest, CollectionTest — and removes the old upsert-swap test that
locked the previous mapping. Reframes the upsert docs and records the
behavioural change in breaking-changes §3.

Assisted-By: AI
@abdul-kaioum

Copy link
Copy Markdown
Member Author

Added one more fix on top: upsert now manages updated_at (commit 9677449). On a $timestamps model, upsert inserts both created_at and updated_at and bumps updated_at = VALUES(updated_at) on duplicate while preserving created_at — replacing the prior unmanaged/VALUES(created_at) behavior.

Unlike the other commits in this PR, this one does change observable behavior (the generated upsert SQL for timestamped models), so it's recorded in breaking-changes.md §3. The non-generic GeminiReviewFixesTest was split into topical suites (UpsertTest/AttributeCastTest/CollectionTest). 113 tests green, 7.4 compat clean.

belongsToMany was a stub: it set a distinct relateAs tag but resolved
identically to hasMany (no junction table). Extend it to a Laravel-style
signature so it loads related rows through a pivot table.

  belongsToMany($model, $pivotTable = null, $foreignPivotKey = null,
      $relatedPivotKey = null, $parentKey = null, $relatedKey = null)

The pivot branch JOINs the pivot table and carries the parent-link column
back as an aliased select column, bucketing related rows on it; eager
with() and lazy access both supported. withPivot() selects extra pivot
columns, exposed flat as pivot_<col> attributes. Aggregates/whereHas over
a pivot relation and withPivot() on a non-pivot relation fail loud.

Zero-BC: each non-pivot path (hasMany, belongsTo/hasOne, legacy
belongsToMany($m) with no pivot table) is byte-identical -- the pivot
logic sits behind early relateAs guards above untouched bodies. PHP 7.4
compatible. Read-side only; attach/detach/sync deferred.

Assisted-By: AI
3e5db3a dropped the leading Connection::wpPrefix() from join() to stop
default-prefix models double-prefixing. That broke custom-$prefix models
(e.g. bit-crm): Model::getPrefix() returns the raw $prefix without wp_, so
join/pivot tables lost the wp_ prefix and pointed at the wrong table.

Add Model::getTablePrefix() -- the full prefix (wp_ plus plugin prefix)
that mirrors how the constructor builds $this->table -- and route the
constructor, join(), and the pivot select/join helper through it.
getPrefix() is unchanged (public API). Default-prefix models stay
byte-identical (getTablePrefix() === getPrefix()); custom-prefix joins and
pivots regain wp_.

Also simplify the pivot helper: applyPivotSelectAndJoin() returns
[pivot, pivotRef, alias] so callers stop recomputing them.

Assisted-By: AI
save()/update() encode array and object attribute values via
wp_json_encode(), but bulkInsert() (insert([[...],[...]])) and upsert()
bound them raw -- an array became the literal "Array" and an object threw
"could not be converted to string". Mirror the save() encoding in both bulk
value builders so callers don't have to wp_json_encode() manually.

Scalars are unchanged; repairs already-broken output (zero-BC).

Assisted-By: AI
…oin fix

- Note that array/object values are JSON-encoded across save/update/insert/
  bulk insert/upsert (usage + breaking-changes §3).
- Note join() prepends the model's full table prefix; add getTablePrefix()
  to the additions list and a §3 behavioral note for the custom-$prefix fix.
- Drop the now-stale pivot limitation claiming custom-$prefix is unsupported.

Assisted-By: AI
prepareColumnName() wrapped the whole "title AS t" string as a single
identifier (`table`.`title AS t`), so select(['col AS alias']) produced
"Unknown column 'table.col AS alias'" errors. Split on ` AS `
(case-insensitive): qualify the column, keep the alias as a separate
back-ticked identifier -> `table`.`col` AS `alias`. Plain/dotted columns and
`*` are unchanged; raw expressions and function calls still need selectRaw().

Assisted-By: AI
with() builds `foreignKey IN ( SELECT * FROM (<parent>) AS subquery )` from a
clone of the parent query narrowed to the local key. select() resets the
select list but not selectRaw, so a parent selectRaw('... as x') leaked a
second column into the subquery -> MySQL "Operand should contain 1 column(s)".

Strip selectRaw on the key-subquery clone via a shared prepareKeySubquery()
helper, used by both the hasMany/belongsTo and pivot eager paths. Byte-identical
when the parent has no selectRaw.

Assisted-By: AI
The `foreignKey IN ( SELECT key ... )` eager subquery cloned the parent and
stripped selectRaw but kept ORDER BY. A parent ordered by a selectRaw alias
(orderBy('dsc') after selectRaw('... as dsc')) then referenced a column absent
from the narrowed subquery -> "Unknown column in order clause"; and ORDER BY is
meaningless for set membership regardless.

Move prepareKeySubquery onto QueryBuilder (so it can reset the protected
orderBy) and drop ORDER BY unless a LIMIT pins which parent rows the set comes
from. Byte-identical for parents without selectRaw/orderBy.

Assisted-By: AI
…tions

41 tests locking verified current behavior (no source change):
- WhereClauseEdgeTest: whereIn type-placeholders, implicit-IN array value,
  IS NULL, falsy values not dropped, operator/LIKE variants, whereBetween,
  nested-closure binding order, whereRaw/selectRaw/having binding order,
  soft-delete scope binding order through the paren-wrap.
- SelectJoinAggregateEdgeTest: column-alias variants (dotted/lowercase/
  backticked/spaced/multi-as), RIGHT/FULL/CROSS join, on()/orOn() chaining,
  custom-$prefix join ON qualification, count()=COUNT(pk), max/min empty=null,
  aggregate-runs-on-clone, asc() pk fallback.
- RelationEagerEdgeTest: eager/whereHas/withCount constraint closures, multiple
  whereHas, withCount alias, select+withCount no dup, relation alias attach,
  parent filter in eager subquery, eager-on-first().

Assisted-By: AI
…ase 1)

Pure repairs of crashing / invalid-SQL paths plus additive forceDelete/restore;
byte-identical for every currently-working input (momus-reviewed).

- insert(): route to bulk only for a true list-of-rows, so a single row whose
  first value is an array no longer crashes via ksort().
- where/whereIn: null + explicit operator -> IS [NOT] NULL; empty array -> the
  false constant `0 = 1` (was invalid `IN ()`); a nested IN element -> one
  JSON-encoded placeholder; object value -> wp_json_encode (was fatal on prepare).
- empty save() (no dirty columns) skips the malformed `UPDATE ... SET` and
  returns the model (fires saving/saved).
- insert([]) / empty bulk rows / upsert($v, []) no longer emit malformed SQL.
- aggregate(fn, '*') -> COUNT(*) (was invalid COUNT(`t`.*)); count() unchanged.
- forceDelete() (real DELETE bypassing the soft scope) and restore() on
  soft-delete models.
- take()/skip() cast to int -> blocks limit/offset injection.
- eager key subquery also strips the parent's groupBy/having.

Assisted-By: AI
…d bulk, schema DDL (zero-BC Phase 2)

Behavior-changing fixes verified safe against bit-pi/bit-crm/bit-social/bit-assist
usage (consumers pass only identifiers to order/group by, don't use the cast
aliases, etc.); byte-identical for currently-working inputs (momus-reviewed).

- E2: orderBy()/groupBy() validate the column is a plain identifier
  (`^[A-Za-z0-9_.`]+$`) and throw otherwise -> blocks ORDER BY/GROUP BY
  injection. Valid identifiers emit byte-identical SQL; raw expressions still go
  through orderByRaw().
- B1: cast aliases integer/float/double/json/datetime now map onto the real
  casters (were silent no-ops).
- C1: relation values (Collection / Model / list-of-Model) are excluded from the
  save/update column set -> lazy relation access no longer corrupts a later
  save(); scalar/JSON-array columns still persist.
- C6: bulk insert aligns each row to the header columns by key (was silent
  misalignment for ragged rows); uniform rows unchanged.
- B4: Schema edit-mode unique() emits `ADD UNIQUE INDEX`; renameColumn() in
  edit() emits `RENAME COLUMN`; decimal($p, $s) works (incl. scale 0).

Assisted-By: AI
An eager-loaded parent with no related rows stored null, so accessing the
relation fell through offsetExists() to a fresh lazy query (the N+1 the eager
load exists to prevent) that returned an empty Collection. Store [] instead:
offsetExists() is true, the resolved-empty value is returned with no extra
query. Empty either way (count 0, falsy); the type now matches a non-empty
eager relation (plain array). No existing test relied on the null.

Assisted-By: AI
A model with $soft_deletes = true now injects deleted_at IS NULL on every
SELECT automatically; trashed rows no longer appear. Opt out with
public $soft_delete_scope = false to restore the unfiltered read.

refresh() reloads a row by its own primary key with withTrashed(), so a
re-hydrated trashed model still reports exists() === true and the next
save() UPDATEs instead of re-INSERTing a duplicate.

Scope is injected on SELECT only; delete/restore/forceDelete/insert/
upsert/aggregate paths are unaffected.

Assisted-By: AI
Add docs/relations.md — a standalone relationship reference covering
hasOne/belongsTo (the shared oneToOne alias and reversed-from-Laravel key
naming), hasMany, belongsToMany pivot relations (signature, key defaults,
withPivot, pivot_* attributes, read-only limits), eager/lazy loading, and
relation aggregates. Linked from README and usage.md; the deep belongsToMany
block and relation Limitations bullets in usage.md now point to it.

Document the soft-delete default flip: usage.md (property table, deleting
section, limitations) and breaking-changes.md (new 2.13 + summary row 13).

Assisted-By: AI
Failing tests pinning the desired behavior for relation-name resolution:
with()/withCount()/whereHas() must reject a non-relation method name with a
RuntimeException, and getActiveRelationKey() must fail loudly on an unknown
relation tag rather than returning a silent null.

These are the RED phase for the not-yet-implemented relation-safety fix
(option alpha) and fail until that guard lands.

Assisted-By: AI
Relation resolution (prepareRelation, the single chokepoint for with/
withCount/whereHas/withWhereHas) previously called any method whose name
matched a relation, gated only by method_exists — letting a framework
Model method (e.g. refresh(), which runs a SELECT) or any consumer no-arg
method run through a relation name, and failing messily on typos.

Now: a name declared on the framework Model is rejected WITHOUT being
called (declaring-class identity via ReflectionMethod, compared to
Model::class so it holds under php-scoper; memoized per class::method); a
consumer-declared method is called and its return validated as a relation
query (QueryBuilder whose model has an active relation key) or rejected
with a RuntimeException. A name that fails method_exists is still silently
skipped (zero-BC for optional/typo'd relation lists).

getActiveRelationKey() now throws on an unknown relation tag instead of
returning a silent null.

Turns the RED RelationResolutionSafetyTest green (216 tests, all pass).

Assisted-By: AI
Add the empirically-confirmed footguns a v1 consumer hits, under their
existing sections:
- 2.1 an is_array($result) "got rows?" guard now inverts (Collection when
  rows exist, [] when empty) -> silent data loss; use empty()/!empty().
- 2.2 $model->update([...])->save() fatals ("save() on false") on a 0-row
  update -> drop the trailing ->save().
- 2.4 a function/expression in select() only "survives" if it contains a
  '.', so CONCAT(url,...) breaks on dotless hosts (localhost); use selectRaw.
- 2.9 a leftover no-arg ->withCount() throws ArgumentCountError, including
  when reached via eager-load relation resolution.

Assisted-By: AI
exec() returns rows-affected (0 for a no-op UPDATE) and false only on a real
DB error or a cancelled pre-event. save()'s existing-model branch checked
exec() for truthiness, so a valid UPDATE that changed no rows (an idempotent
re-save where no value differs) returned false — misreporting success as
failure. This was the root cause of `$model->update([...])->save()` fataling
(false->save()) and made `if (!$model->save())` falsely error on no-op saves.

Compare against false so any non-error result (including 0 rows) returns the
Model and fires 'saved', consistent with the 'updated' event that already
fired. Insert path and genuine-error path unchanged.

Documented in breaking-changes.md §3 (+ §2.2 note updated). Adds
SaveZeroRowUpdateFixTest (0-row returns Model, chained update()->save() no
fatal, real error still false).

Assisted-By: AI
save()'s insert branch decided success by lastInsertId() ($wpdb->insert_id),
so a successful INSERT into a table with a manual/composite key (insert_id 0)
returned false and never fired 'saved'. It also risked returning the Model on
a FAILED insert when insert_id held a stale non-zero value from an earlier
insert in the same request.

Decide success from exec() (false only on real error/cancel), then still
assign the auto-increment id to the primary key when present. A manual/
composite-key insert now returns the Model; a genuine error returns false
before touching lastInsertId (no stale PK, no 'saved').

Documented in breaking-changes.md §3. Adds SaveInsertReturnFixTest
(auto-increment sets PK; manual-PK returns Model; error returns false).

Assisted-By: AI
Add the edge cases the session's fixes introduced but left unverified:
- soft-delete: refresh() reloads a trashed row without the scope filter
  (withTrashed) so exists() stays true.
- relation guard: a method returning a plain QueryBuilder with no active
  relation key is rejected; a framework Model method is rejected; the 4th
  entry point withWhereHas() rejects a non-relation too.
- save() insert: a failed insert carrying a stale non-zero insert_id returns
  false and does not set the PK from it.
- saved event fires on a 0-row UPDATE and a manual-PK INSERT, and never on a
  failed write (new SavedEventUser fixture).

getActiveRelationKey() short-circuits a null relation tag before the isset()
so it no longer trips PHP 8's "null as array offset" deprecation (surfaced by
the plain-QueryBuilder rejection test). Behavior unchanged — both still throw.

Assisted-By: AI
…, events)

Cover the three edges previously flagged as skipped:
- relation guard memoizes the framework-vs-consumer verdict per class::method
  (reflection-asserted: refresh -> true, posts -> false, no collision).
- a relation declared on an intermediate base class (leaf -> base -> Model)
  resolves, since its declaring class is the base, not the framework Model.
- created fires on a successful INSERT and updated fires even on a 0-row
  UPDATE (SavedEventUser gains created/updated counters).

New fixtures RelationBaseModel/RelationLeafModel. The memoization test guards
ReflectionProperty::setAccessible() behind PHP_VERSION_ID (required on 7.4, a
deprecated no-op on 8.1+) to keep the suite deprecation-clean.

Assisted-By: AI
Raise the minimum PHP from ^7.4 || ^8.0 to >=8.2 (breaking: the package no
longer installs on PHP < 8.2). Add phpunit/phpunit ^11.5 as a dev dependency
and a `composer test` script so tests run from the vendored PHPUnit instead of
a gitignored phpunit.phar. Point the compat gate at 8.2- and pin
config.platform.php to 8.2.0 for reproducible installs.

Documented in breaking-changes.md §2.14 (+ summary row 14).

Assisted-By: AI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant